Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Name unions #1027

Merged
merged 8 commits into from
Nov 18, 2021
Merged

Name unions #1027

merged 8 commits into from
Nov 18, 2021

Conversation

swallez
Copy link
Member

@swallez swallez commented Nov 17, 2021

Most strongly typed languages need each member of a union type to have an identifier. In Java we generate custom wrappers for unions, Rust needs names for enum variants (Rust unifies enums and tagged unions).

This PR updates the spec to change inline unions used in property declarations to a distinct type with a new @codegen_names (plural) annotation that names the union members.

class Foo {
  field: Bar | Baz
}

becomes

class Foo {
  field: SomeBarBaz
}

/** @codegen_names bar, baz */
type SomeBarBaz = Bar | Baz

Some inline unions are left in the spec, that represent the leniency rules that ES implements. These are notably SomeType | SomeType[] (read a single object as a 1-sized array) and strings that accept any primitive type.

These leniency rules are currently implemented in the Java generator and will later be formalized as new model validation rules that will report inline unions that require a separate type definition.


Along with refactoring enumerations to enum types, this PR also fixes a number of definitions that were ambiguous or actually wrong. API validation has been run on most of these changes to verify that there were no regressions.

Validation of the search API has 3 errors less, and the remaing ones are because of the use of numbers for ids (they're not quoted in the YAML tests) and experimental bucket_correlation and bucket_count_ks_test aggregations that have not been represented in the spec.

@swallez swallez marked this pull request as ready for review November 17, 2021 21:40
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I would add a compiler check that verifies that if codegen_names is present, then the number of identifiers should be the same as the union members.

@swallez
Copy link
Member Author

swallez commented Nov 18, 2021

@delvedor check added in 48d3699 (it's already checked in the Java generator, but should indeed definitely be checked at spec compile time).

Also added a unification of redundant DocValueField and FieldAndFormat in f88c637

Copy link
Contributor

@philkra philkra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left two questions, not sure if they're applicable, though!

specification/_types/common.ts Outdated Show resolved Hide resolved
specification/_types/mapping/dynamic-template.ts Outdated Show resolved Hide resolved
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@swallez swallez merged commit 94eff4a into main Nov 18, 2021
@swallez swallez deleted the name-unions branch November 18, 2021 09:36
swallez added a commit that referenced this pull request Nov 18, 2021
* Add "@codegen_names" jstag to identify union members
* Extract unions to named types, and fix some type declarations
* Add boolean/enum leniency to the TS generator
swallez added a commit that referenced this pull request Nov 18, 2021
* Add "@codegen_names" jstag to identify union members
* Extract unions to named types, and fix some type declarations
* Add boolean/enum leniency to the TS generator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants